Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(cve): bump busboy to fix CVE-2022-24434 #1097

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

jlourenc
Copy link

@jlourenc jlourenc commented May 21, 2022

This PR bumps busboy to at least 1.0.0 to remove dicer from the transitive dependencies as it contains a denial of service vulnerability: https://security.snyk.io/vuln/SNYK-JS-DICER-2311764.

The remaining of the PR is about adapting the code to the breaking changes introduced in busboy with the bump to v1 and fixing the tests:

  • reverted lib: work around node.js bug #205 to fix should report errors from busboy parsing test as the error was no longer forwarded. I have not really looked into the details but reverting this workaround for an issue related to Node v0 seems like a win anyway,
  • updated should handle unicode filenames test to have a more pertinent example of a unicode filename using both filename and filename* Content-Disposition directives.

This PR supersedes: #1096, #1092 and #1056.

@LinusU
Copy link
Member

LinusU commented May 23, 2022

As far as I'm aware, this would be a breaking change, right? I think we need to do this in 2.x instead?

@jlourenc
Copy link
Author

If the aim is to keep supporting Node v0 which is end-of-life since 2016, then yes it is a breaking change. However, such an aim is, in my opinion, counter-productive. Unless you're considering something else a breaking change?

Also, I've tried and run npm run test using node v0.12.12 and the project is already not compatible with Node v0 I believe:

$ node -v
v0.12.12

$ npm run test

> [email protected] test /Users/jeremylourenco/Code/multer
> mocha --harmony

/Users/jeremylourenco/Code/multer/node_modules/busboy/lib/index.js:3
const { readdirSync } = require('fs');
      ^
SyntaxError: Unexpected token {
    at exports.runInThisContext (vm.js:73:16)
    at Module._compile (module.js:443:25)
    at Object.Module._extensions..js (module.js:478:10)
    at Module.load (module.js:355:32)
    at Function.Module._load (module.js:310:12)
    at Module.require (module.js:365:17)
    at require (module.js:384:17)
    at Object.<anonymous> (/Users/jeremylourenco/Code/multer/lib/make-middleware.js:2:14)
    at Module._compile (module.js:460:26)
    at Object.Module._extensions..js (module.js:478:10)
    at Module.load (module.js:355:32)
    at Function.Module._load (module.js:310:12)
    at Module.require (module.js:365:17)
    at require (module.js:384:17)
    at Object.<anonymous> (/Users/jeremylourenco/Code/multer/index.js:1:84)
    at Module._compile (module.js:460:26)
    at Object.Module._extensions..js (module.js:478:10)
    at Module.load (module.js:355:32)
    at Function.Module._load (module.js:310:12)
    at Module.require (module.js:365:17)
    at require (module.js:384:17)
    at Object.<anonymous> (/Users/jeremylourenco/Code/multer/test/disk-storage.js:9:14)
    at Module._compile (module.js:460:26)
    at Object.Module._extensions..js (module.js:478:10)
    at Module.load (module.js:355:32)
    at Function.Module._load (module.js:310:12)
    at Module.require (module.js:365:17)
    at require (module.js:384:17)
    at /Users/jeremylourenco/Code/multer/node_modules/mocha/lib/mocha.js:231:27
    at Array.forEach (native)
    at Mocha.loadFiles (/Users/jeremylourenco/Code/multer/node_modules/mocha/lib/mocha.js:228:14)
    at Mocha.run (/Users/jeremylourenco/Code/multer/node_modules/mocha/lib/mocha.js:514:10)
    at Object.<anonymous> (/Users/jeremylourenco/Code/multer/node_modules/mocha/bin/_mocha:480:18)
    at Module._compile (module.js:460:26)
    at Object.Module._extensions..js (module.js:478:10)
    at Module.load (module.js:355:32)
    at Function.Module._load (module.js:310:12)
    at Function.Module.runMain (module.js:501:10)
    at startup (node.js:129:16)
    at node.js:814:3

I'm not a Node expert but it seems "Object Destructuring" is a v6+ feature.

@erano067
Copy link

Hi, why not update to busboy atleast version ^1.6. 0?

@jlourenc
Copy link
Author

Hi, why not update to busboy atleast version ^1.6. 0?

The aim of this PR is to remove dicer from the dependencies, which is achieved by upgrading to at least v1.0.0. There is no reason to enforce a higher version as far as this PR is concerned. You may chose to enforce a higher version in your project pulling multer.

@jlourenc
Copy link
Author

I have now enforced the node engine to be >= 6.0.0.

@vaibhavkumar-sf
Copy link

Hi, Thanks for all the updates, can you please bump the busboy version to the latest 1.6.0 otherwise we may need to upgrade it again sooner for other vulnerabilities in the near future?

@Bozzzzzo

This comment was marked as resolved.

@harshagarwal00

This comment was marked as spam.

@ansonallard

This comment was marked as spam.

@vaibhavkumar-sf

This comment was marked as resolved.

@achilehero

This comment was marked as spam.

@kutluturk

This comment was marked as spam.

@dizhenlyu

This comment was marked as spam.

@zacharytyhacz

This comment was marked as spam.

@eran10

This comment was marked as spam.

@senpai-notices
Copy link

senpai-notices commented May 26, 2022

lgtm. i approve this mr. do let any of us know if we can help you with any blockers.

@zSakuraEvilz

This comment was marked as spam.

@progsmile

This comment was marked as spam.

@NormandoHall

This comment was marked as spam.

@Dany-C
Copy link

Dany-C commented May 27, 2022

This PR bumps busboy to at least 1.0.0 to remove dicer from the transitive dependencies as it contains a denial of service vulnerability: https://security.snyk.io/vuln/SNYK-JS-DICER-2311764.

The remaining of the PR is about adapting the code to the breaking changes introduced in busboy with the bump to v1 and fixing the tests:

  • reverted lib: work around node.js bug #205 to fix should report errors from busboy parsing test as the error was no longer forwarded. I have not really looked into the details but reverting this workaround for an issue related to Node v0 seems like a win anyway,
  • updated should handle unicode filenames test to have a more pertinent example of a unicode filename using both filename and filename* Content-Disposition directives.

This PR supersedes: #1096, #1092 and #1056.

My NestJs app didn't compile, so i found this temporary fix:

As of npm cli v8.3.0 (2021-12-09) this can be solved using the overrides field of package.json

For NestJs :
overrides": { "@nestjs/platform-express": { "multer": { "busboy": "1.0.0" } } }

For Express :
overrides": { "multer": { "busboy": "1.0.0" } }

@vaibhavkumar-sf
Copy link

override is a temporary solution to use until the package owner does not upgrade the package, also override is okay only if you are sure that the required version will not break the package @Dany-C

31453 added a commit to arkime/arkime that referenced this pull request May 31, 2022
used by pcap upload
because of CVE-2022-24434
more info: expressjs/multer#1097
@LinusU
Copy link
Member

LinusU commented Jun 1, 2022

@EstartuPrime how was your dependency on Multer specified? As far as I understand it shouldn't automatically upgrade to 1.4.5-lts.1, since it has the -lts.1 postfix.

@EstartuPrime
Copy link

@LinusU You're right, we updated to 1.4.5-lts.1 manually as we noticed the deprecation information and read in youre deprication wraning, that this 1.4.5-lts.1 still supports Node 6. But during startup of the application it crashes when busboy is loaded by the reason i linked to busboy.

We switched back to 1.4.4 and the application is back to normal.

All this took me some time to find out where exactly the problem is and if we can solve it anyway and can keep the security fix.
And as i understand in the linked Question, this "issue" will not be fixed in Busboy.
That's why i thought i better informed you about this incompatibility with Node.js 6 and busboy 1.6.0 , so that you may update your'e deprecation warning.

@RopoMen
Copy link

RopoMen commented Jun 2, 2022

Hi,
I think that better fix to unicode issue can be achieved through this PR #1102

@zoeesilcock
Copy link

Another package that is affected by this is multer-gridfs-storage, see this issue. That package already requires Node 12 so it shouldn't require a major step for them. I would like to provide a PR for that package, am I correct in thinking that the solution for packages in this situation is to change their peer dependecy so it is pinned to 1.4.5-lts.1? Or would ^1.4.5-lts.1 work to allow future versions?

@ghost91-
Copy link

The latter should work, too.

Jyrno42 added a commit to thorgate/tg-resources that referenced this pull request Aug 11, 2022
robertdeniszczyc2 added a commit to UKHomeOffice/hocs-frontend that referenced this pull request Aug 15, 2022
Updates multer and nodemon to resolve security vulnerabilities.
multer is updated to a semver convention breaking version due to no
fix for the dependancy of multer causing the vulnerability.

More details for this: expressjs/multer#1097
robertdeniszczyc2 added a commit to UKHomeOffice/hocs-frontend that referenced this pull request Aug 15, 2022
Updates multer and nodemon to resolve security vulnerabilities.
multer is updated to a semver convention breaking version due to no
fix for the dependancy of multer causing the vulnerability.

CVE for the issue: CVE-2022-24434

More details for this: expressjs/multer#1097
@alasdairhurst
Copy link

From what i can see, busboy 1.0.0+ has documented a requirement of node >=10.16.0. Is this an oversight or is there a reason this is being ignored and the engines in multer being set to a lower version (6)?

mcdurdin added a commit to keymanapp/keyman that referenced this pull request Sep 19, 2022
Removes Keyman Developer Server's transitive dependency on dicer by
updating multer to `1.4.5-lts.1`, which updates its dependency on
busboy.

See
expressjs/multer#1097 (comment)
for reasoning behind use of  `-lts.1` rather than a full release
version.

At some point in the future, multer will publish a full release with
this fix, at which point we can move back to a full release version.
@hongyanh
Copy link

npm uninstall --save multer and npm install --save multer fixed my problem.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.